Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(APIController): Allow self-test depending on token and not user-agent #2175

Merged
merged 1 commit into from
Jan 31, 2025
Merged

fix(APIController): Allow self-test depending on token and not user-agent #2175

merged 1 commit into from
Jan 31, 2025

Conversation

provokateurin
Copy link
Member

I would also like to use this endpoint without having a matching user-agent.
The token is a better method to verify the client can even handle push notifications, because in order to have a valid token it already needs to be registered for push notifications.

@provokateurin provokateurin added this to the Nextcloud 32 milestone Jan 30, 2025
@provokateurin
Copy link
Member Author

/backport to stable31

@provokateurin provokateurin mentioned this pull request Jan 30, 2025
Open
@github-actions GitHub Actions
Copy link
Contributor

🐢 Performance warning.
It looks like the query count of the integration tests increased with this PR.
Database query count is now 8909 was 8700 (+2.4%)
Please check your code again. If you added a new test this can be expected and the base value in tests/Integration/base-query-count.txt can be increased.

@provokateurin provokateurin force-pushed the fix/apicontroller/self-test-token branch from 7e857a4 to 1634fdc Compare January 30, 2025 16:27
@github-actions GitHub Actions
Copy link
Contributor

🐢 Performance warning.
It looks like the query count of the integration tests increased with this PR.
Database query count is now 8909 was 8700 (+2.4%)
Please check your code again. If you added a new test this can be expected and the base value in tests/Integration/base-query-count.txt can be increased.

@nickvergessen nickvergessen force-pushed the fix/apicontroller/self-test-token branch from 1634fdc to 4c80910 Compare January 31, 2025 15:25
@github-actions GitHub Actions
Copy link
Contributor

🐢 Performance warning.
It looks like the query count of the integration tests increased with this PR.
Database query count is now 8909 was 8700 (+2.4%)
Please check your code again. If you added a new test this can be expected and the base value in tests/Integration/base-query-count.txt can be increased.

@nickvergessen
Copy link
Member

The token is a better method to verify the client can even handle push notifications

The check was incomplete … It would have needed to extend to check if it's actually a permanent token and from there we should then have checked notifications_pushhash in addition and I don't think that's worth it for now, while we can't later on limit to which devices we want to push. So for now we can remove it.
I would bring it back and limit it to push the the requested device in the future.

@nickvergessen nickvergessen merged commit 75d67a3 into master Jan 31, 2025
47 checks passed
@nickvergessen nickvergessen deleted the fix/apicontroller/self-test-token branch January 31, 2025 15:30
@backportbot backportbot bot mentioned this pull request Jan 31, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

Assignees
No one assigned
Labels
3. to review bug
Projects
None yet
Milestone
Nextcloud 32
Development

Successfully merging this pull request may close these issues.
2 participants
@provokateurin @nickvergessen